Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce new Android artifact distribution strategy #32

Merged
merged 4 commits into from
Dec 29, 2023

Conversation

complexspaces
Copy link
Collaborator

@complexspaces complexspaces commented Sep 8, 2023

The goal of this pull request is to resolve the outstanding issues with the current Android component distribution system and provide a new one that checks as many boxes as possible without causing issues elsewhere.

To accomplish this, a new method has been built to completely replace the old way. For the exact details, see the documentation included in the changes. At a high level, we've switched to distributing pre-compiled versions of the Android component via a Maven local repository. A new crate, rustls-platform-verifier-android, is responsible for containing these artifacts. Upon downloading this new crate via cargo, Gradle is able to seamlessly locate the files inside Cargo's dependency cache and compile the packaged AAR into the main application's build.

This approach eliminates the toolchain synchronization problems and poor Gradle workarounds that affected the last approach. Additionally, it has minimal-to-no affect on Gradle build or configuration performance and fits in much better with its compilation model.

Further Improvements

See #33, which builds upon this packaging logic to provide always-available Android artifacts based off the current main. The idea behind this is to attempt to preserve the convenience of the Git dependencies that cargo supports. Depending on the release cadence here in the future, people may need to depend on hot-fixes for short periods. By hosting these pre-compiled artifacts in a dedicated, opt-in branch, those with Android apps can add a Git dependency on main-with-maven instead to avoid the need to temporarily vendor AAR artifacts.

@complexspaces complexspaces added the O-Android Work related to the Android verifier implementation label Sep 8, 2023
@complexspaces complexspaces added this to the 0.1 crates.io release milestone Sep 8, 2023
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work!

rustls-platform-verifier/Cargo.toml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
admin/RELEASING.md Outdated Show resolved Hide resolved
@complexspaces complexspaces force-pushed the android-artifact-distribution branch 2 times, most recently from a2a2ce3 to 71a58d0 Compare September 9, 2023 03:37
@complexspaces complexspaces changed the title WIP: Introduce new Android artifact distribution strategy Introduce new Android artifact distribution strategy Sep 9, 2023
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice work! Thank you for the detailed descriptions of why this option makes the most sense. Especially the parts about the discarded alternatives. It's very helpful background.

I flagged a handful of typos and some minor scripting things but none feel blocking.

README.md Outdated Show resolved Hide resolved
admin/RELEASING.md Outdated Show resolved Hide resolved
admin/RELEASING.md Outdated Show resolved Hide resolved
admin/RELEASING.md Show resolved Hide resolved
admin/RELEASING.md Show resolved Hide resolved
android-release-support/src/lib.rs Outdated Show resolved Hide resolved
android-release-support/src/lib.rs Outdated Show resolved Hide resolved
admin/RELEASING.md Show resolved Hide resolved
ci/package_android_release.sh Outdated Show resolved Hide resolved
ci/package_android_release.sh Outdated Show resolved Hide resolved
@cpu
Copy link
Member

cpu commented Nov 20, 2023

@complexspaces WDYT about rebasing this and merging it as-is (maybe modulo the small fixes called out by tshepang)? I think my feedback about more documentation for the release process doesn't need to be blocking. We could iterate on those parts based on experience.

@cpu
Copy link
Member

cpu commented Dec 4, 2023

@complexspaces small bump. Would it be helpful if I rebased this branch and addressed tshepang's comments?

@complexspaces
Copy link
Collaborator Author

Hi, sorry for the delay @cpu. I've once again gotten behind on things. I will address tshepang's comments this week and try to clean up some of the other smaller comments as well before Monday next week. At that point, we can evaluate if this looks good enough for an initial publishing attempt?

@cpu
Copy link
Member

cpu commented Dec 6, 2023

I will address tshepang's comments this week and try to clean up some of the other smaller comments as well before Monday next week.

Great, thank you! I know it's a busy time of year :-)

At that point, we can evaluate if this looks good enough for an initial publishing attempt?

Sounds good to me. I'm eager to get something out into the world before the new year if we can convince ourselves that all of our ducks are in a row. It might be nice to land #42 but it seems unlikely that Reqwest and the other blocking dependencies will be ready on a short timescale. The very recent Hyper 1.0 update layers a bit more complication into the mix.

@cpu
Copy link
Member

cpu commented Dec 13, 2023

Checking in again: anything I can do to help move closer to publishing a release?

@cpu
Copy link
Member

cpu commented Dec 21, 2023

Here's an updated branch that's been rebased on main and had the majority of the outstanding review feedback addressed. I could force push it onto this branch, or open a new PR as folks prefer.

@complexspaces complexspaces force-pushed the android-artifact-distribution branch 4 times, most recently from 29d6cda to b6fe594 Compare December 25, 2023 08:04
@complexspaces
Copy link
Collaborator Author

I believe that I've finished this branch and it's ready for another review. It took a bit longer then expected (1-2 days last week), and I apologize for the inconsistency. I ended up needing to port the .gradle script examples to Kotlin in order to end-to-end test the release/dependency process. 1Password internally moved to .gradle.kts files, and that was also my "test user" project :)

In the extra time I also discovered some Gradle performance/correctness improvements as well so IMO the initial release will be more polished as a result.

New changes include:

  • Fixing up the typos that were pointed out, and more I noticed after temporarily enabling a spellchecker.
  • Cherry-picking in and then squashing @cpu's release documentation suggestions.
  • Resolving the last of the threads I believed were important (notably the context around --allow-dirty, and documenting a future TODO that may remove the need for it so no one forgets)
  • Updating the Gradle integration example to remove the hacky Memoize usage and instead place the Maven repository definition in the right place.
    • This saves on configuration time and ensures the script is only run once the Gradle-intended way 😅.
  • Documented intended Gradle sync performance/behavior so regressions have a baseline to report against.
  • Added steps for verifying the rustls-platform-verifier-android dry-run package both on the filesystem and inside a Gradle project for testing.

Using all of the now-documented steps, I tested a few things to build confidence in the first release we make:

  • The Gradle integration is working as expected WRT caching and version updates (using a filesystem rustls-platform-verifier dependency patch)
  • Filesystem crate patches are recognized for local development/overrides in both Cargo and Gradle.
  • The extracted dry-run crate package is recognized by Gradle as well.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The latest revisions look good to me 🚀

@complexspaces complexspaces merged commit ca41ec3 into main Dec 29, 2023
14 checks passed
@complexspaces complexspaces deleted the android-artifact-distribution branch December 29, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-Android Work related to the Android verifier implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants